Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| expect(backgroundColor).toBe('rgb(218, 216, 255)'); | ||
| }); | ||
|
|
||
| test('should be able to customize month/year picker part within the grid style', async ({ page }, testInfo) => { |
There was a problem hiding this comment.
We didn't have a test for this so I added it.
ShaneK
left a comment
There was a problem hiding this comment.
This looks good to me! Awesome work!
brandyscarney
left a comment
There was a problem hiding this comment.
Looks good! A few requests & questions.
| aria-label="Previous month" | ||
| disabled={prevMonthDisabled} | ||
| onClick={() => this.prevMonth()} | ||
| part="prev-next-buttons prev-button" |
There was a problem hiding this comment.
I'm torn on the prev-next-buttons naming because usually we just name it a shared name like in the components that have helper text & error text we call them supporting-text:
<div class="helper-text" part="supporting-text helper-text">{helperText}</div>
<div class="error-text" part="supporting-text error-text">{errorText}</div>I would think we would do something similar here like:
| part="prev-next-buttons prev-button" | |
| part="navigation-button previous-button" |
I spelled out previous because using full words improves readability and makes translations easier.
There was a problem hiding this comment.
Navigation buttons sound better! 8dc99f4
| aria-label="Next month" | ||
| disabled={nextMonthDisabled} | ||
| onClick={() => this.nextMonth()} | ||
| part="prev-next-buttons next-button" |
There was a problem hiding this comment.
| part="prev-next-buttons next-button" | |
| part="navigation-buttons next-button" |
See my previous comment
| expect(backgroundColor).toBe('rgb(255, 165, 0)'); | ||
| }); | ||
|
|
||
| test('should be able to customize calendar header part within the grid style', async ({ page }) => { |
There was a problem hiding this comment.
Do we need to say within the grid style on all of these?
| expect(backgroundColor).toBe('rgb(173, 216, 230)'); | ||
| }); | ||
|
|
||
| test('should be able to customize prev/next buttons part within the grid style', async ({ page }, testInfo) => { |
There was a problem hiding this comment.
Could we combine this test with the previous & next button part tests? Maybe the one targeting both could apply background color while the individual parts could apply text color.
| ` | ||
| <style> | ||
| ion-datetime::part(days-of-week) { | ||
| background-color: #9ad162; |
There was a problem hiding this comment.
Nit: Can we stick with named colors to make it easier to verify the rgb matches up?
There was a problem hiding this comment.
Why don't we expose datetime-selected-date?
| </div> | ||
| </div> | ||
| <div class="calendar-days-of-week" aria-hidden="true"> | ||
| <div class="calendar-days-of-week" aria-hidden="true" part="days-of-week"> |
There was a problem hiding this comment.
Do you think we should call this calendar-days-of-week to match the naming of calendar-header? I am wondering if it's confusing that we have calendar-day and calendar-header but then stop prefixing with calendar. We probably can't enforce this though without causing breaking changes since month-year-button is released already.
ion-datetime,part,calendar-day
ion-datetime,part,calendar-header
ion-datetime,part,datetime-header
ion-datetime,part,days-of-week
ion-datetime,part,month-year-button
ion-datetime,part,next-button
ion-datetime,part,prev-button
ion-datetime,part,prev-next-buttons
ion-datetime,part,time-button
ion-datetime,part,wheel
ion-datetime,part,wheel-item
There was a problem hiding this comment.
I'm not against it. The original thought was trying to keep it short but I'm convinced to keep it consistent. We should reconsider the naming during ionic modular work. 0bc7bf3
| * layout with `presentation="date-time"` or `"time-date"`. | ||
| * @part time-button active - The time picker button when the picker is open. | ||
| * | ||
| * @part calendar-header - The calendar header manages the date navigation controls (month/year picker and prev/next buttons) and the days of the week when using a grid style layout. |
There was a problem hiding this comment.
| * @part calendar-header - The calendar header manages the date navigation controls (month/year picker and prev/next buttons) and the days of the week when using a grid style layout. | |
| * @part calendar-header - The calendar header manages the date navigation controls (month/year picker and previous/next buttons) and the days of the week when using a grid style layout. |
Co-authored-by: Brandy Smith <6577830+brandyscarney@users.noreply.github.com>
Co-authored-by: Brandy Smith <brandyscarney@users.noreply.github.com>
Co-authored-by: Brandy Smith <6577830+brandyscarney@users.noreply.github.com>
Co-authored-by: Brandy Smith <6577830+brandyscarney@users.noreply.github.com>
Issue number: resolves #30083, resolves #30830
What is the current behavior?
There's no way to customize the header when using a grid style. This includes the entire header, prev/next buttons, and the days of the week container. The only section that can customized within the header is the month/year picker. This limits developers from being able to make changes to match their styles.
What is the new behavior?
Does this introduce a breaking change?
Other information
Preview